-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set HTTP hostname based on TLS server name #350
base: main
Are you sure you want to change the base?
Conversation
9c24fa6
to
038724f
Compare
Hello, Thank you for your contribution. I am not willing to have this repository depend on client_golang. We have an exception for an interface in version, and the tests in sigv4, but that's it. |
The net/http library uses the Host field from the Request object in order to determine the value of the Host header [1]. In order for the Prometheus client to support SNI, it needs to set this field to the value provided in the TLS server name. [1] golang/go#29865 Signed-off-by: fpetkovski <[email protected]>
038724f
to
c81c25d
Compare
Thanks for the feedback. Could you point me to where the dependency to client_golang is introduced in this PR? Do you mean because we are referencing the TLS config? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I don't see any client_golang dep @roidelapluie 🤔
Looks good to me, but let's wait for @roidelapluie voice, maybe I am missing something.
CertFile: ClientCertificatePath, | ||
KeyFile: ClientKeyNoPassPath, | ||
ServerName: "test-domain.com", | ||
InsecureSkipVerify: true}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InsecureSkipVerify: true}, | |
InsecureSkipVerify: true, | |
}, |
The initial commit introduced a dependency. Then it was force pushed. I can not merge this as is, I need to check that 1. SNI works even with our connection pooling and 2. This does not break blackbox exporter, which has similar logic. |
Ah yes, that seems to be correct: 038724f#diff-cdb28b391884a82ef481177749e35ba9c3c54bda95d93ffe3d5f3ba1c67c2c5eR35 |
after thinking twice, the TLS servername should come from the header, not the other way around. |
What do you mean by this? Isn't the TLS server name a user provided config? |
The net/http library uses the Host field from the Request object in
order to determine the value of the Host header [1]. In order for the
Prometheus client to support SNI, it needs to set this field to the
value provided in the TLS server name.
[1] golang/go#29865
Signed-off-by: fpetkovski [email protected]
Fixes prometheus/prometheus#9403